fix(cli): guard customDeepMerge against circular references#28387
fix(cli): guard customDeepMerge against circular references#28387vedhakoushik wants to merge 1 commit into
Conversation
customDeepMerge's mergeRecursively recursed into nested plain objects with no cycle tracking, so a settings object containing a circular reference (e.g. obj.self = obj) caused unbounded recursion and crashed the settings manager with RangeError: Maximum call stack size exceeded, breaking CLI initialization until the user manually scrubbed their config. The repo already documented this with a test asserting the crash. Track the chain of source-to-clone mappings currently being merged in a Map. When a source value is a circular reference back to an ancestor, assign the ancestor's clone rather than recursing, so the merged result is a fully independent clone (result.self === result) and never embeds a live reference back into the caller's input. Shared-but-non-circular references (DAGs) are unaffected and still merge normally. Add regression tests for self, mutual (a->b->a), and shared-reference cases, and flip the settings.test.ts circular-reference test from asserting the crash to asserting it no longer throws. Fixes google-gemini#28270 Signed-off-by: Bellam vedha kousik <bellam.vedhakoushik@gmail.com>
|
📊 PR Size: size/M
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the customDeepMerge utility to handle circular references (both self-referential and mutual) without throwing a stack overflow error. It achieves this by tracking the chain of source objects being merged using a clones Map, resolving circular references to their respective clones, and cleaning up the map on recursion exit to allow normal merging of shared but non-circular references. Corresponding unit tests have been added and updated to verify this behavior. There are no review comments to address.
gemini-cli
Bot
added
priority/p2
1 participant
What
Fixes #28270.
customDeepMerge'smergeRecursivelyrecursed into nested plain objects with no cycle tracking, so a settings object containing a circular reference (e.g.obj.self = obj) caused unbounded recursion and crashed the settings manager withRangeError: Maximum call stack size exceeded, breaking CLI initialization until the user manually scrubbed their config.The repo already documented this limitation with a test in
settings.test.tsthat asserted the crash (.toThrow(/Maximum call stack size exceeded/)).Fix
packages/cli/src/utils/deepMerge.ts— track the chain of source-to-clone mappings currently being merged in aMap, added on entry and removed on exit so it represents only the current ancestor path (not already-processed siblings). When a source value is a circular reference back to an ancestor, assign the ancestor's clone rather than recursing, so the merged result is a fully independent clone (result.self === result) and never embeds a live reference back into the caller's input.This is deliberately scoped so that shared-but-non-circular references (DAGs) still merge normally — only true cycles are short-circuited.
Tests
deepMerge.test.ts: added regression tests for self-reference (obj.self = obj), mutual/indirect cycles (a → b → a), and a shared non-circular reference (DAG) to guard against over-eager short-circuiting.settings.test.ts: flipped the existing circular-reference test from asserting the crash to asserting it no longer throws.Verification
deepMerge.test.ts— 20/20 pass (4 new)settings.test.tscircular-reference test — passeseslint,tsc --noEmit, andprettier --checkall clean on the changed filesSupersedes #28349, opened as a clean re-submission — the original PR's commits carried a
Co-Authored-Bytrailer that blocked the CLA check from resolving. Same fix, same tests, no code changes; only the commit metadata differs.